Add Format and Lint to Makefile #545
Conversation
woop
requested review from
davidheryanto,
khorshuheng,
pradithya and
zhilingc
as code owners
|
I think we still need two or three things before we can close #526.
|
|
/test test-python-sdk |
|
The latest commit also removes Python proto generated code. |
| pre-commit | ||
| flake8 | ||
| black |
There was a problem hiding this comment.
would propose to set the version for the linters like grpcio-tools, mypy, black, flake8 etc.
It would be awesome when we can be sure on same version when developing
Rest is LGTM :) The import structure and linter, format would be really helpful for anyone developing next
There was a problem hiding this comment.
I agree, but we also dont want to be too restrictive. If we force everyone to conform to our versions then its also quite high friction. Only protoc was a problem, and with the addition of this PR we won't be version controlling proto generated Python code, so the problem kind of goes away.
I think we can pin the versions once it becomes a problem
There was a problem hiding this comment.
The "compatible" specifier might be a reasonable compromise, e.g. approximately expecting Semantic Versioning if a tool doesn't break behavior within a major version series:
flake8 ~= 3.0
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: imjuanleonard, woop The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| cd ${ROOT_DIR}/sdk/go; gofmt -s -w *.go | ||
|
|
||
| lint-go: | ||
| cd ${ROOT_DIR}/sdk/go; go vet; golint *.go |
There was a problem hiding this comment.
If my version of golint is still current, it requires a flag -set_exit_status in order to return an error status to the shell.
A formatting check should probably be included in lint-go as well, it is for the other languages. gofmt has no such feature at all to return an error exit status, the idiom seems to be to use test -z $(gofmt -l ./) when this is desired.
I'm not sure if we're actually wanting it to fail on CI yet, because of the desire to not fail lint checks one-at-a-time, but we probably want to be consistent about whether all the lint-* make targets exit nonzero when there's an issue (I haven't checked behavior of all others yet).
Probably lint-go will not fail CI for now even if this target is updated to exit nonzero, because .prow/scripts/test-golang-sdk.sh does not set -e.
* Add Make commands for format, lint, flake8, spotless, isort, black, and refactor * Add mypy test * Add lint tests to CI * Fix broken Python test * Fix broken test for Python * Add black to dependencies * Remove Python Protos * Add automatic local linting * Update precommit names * Add black exclusions * Add tensorflow metadata proto generation * Ignore tf meta directory * Add build essentials to install make in CI * Add exports back to __init__.py * Add __all__ to export * Add white space to export * Add source to export * Fix python export formatting
* Add Make commands for format, lint, flake8, spotless, isort, black, and refactor * Add mypy test * Add lint tests to CI * Fix broken Python test * Fix broken test for Python * Add black to dependencies * Remove Python Protos * Add automatic local linting * Update precommit names * Add black exclusions * Add tensorflow metadata proto generation * Ignore tf meta directory * Add build essentials to install make in CI * Add exports back to __init__.py * Add __all__ to export * Add white space to export * Add source to export * Fix python export formatting
What this PR does / why we need it:
Refactors the root Make file so that we can
formatandlintfor Java, Python, and Golang.Which issue(s) this PR fixes:
This PR works towards fixing #526, but does not address all the necessary requirements to close it.
Does this PR introduce a user-facing change?: